Skip to content

[tests] tighten keep_in_32 modules tests - #14399

Merged
sayakpaul merged 6 commits into
mainfrom
tighten-keep32-modules
Aug 13, 2026
Merged

[tests] tighten keep_in_32 modules tests#14399
sayakpaul merged 6 commits into
mainfrom
tighten-keep32-modules

Conversation

@sayakpaul

@sayakpaul sayakpaul commented Aug 6, 2026

Copy link
Copy Markdown
Member

What does this PR do?

As the title suggests. _keep_in_fp32_modules is an important model class-level attribute that deserves better functional testing. In particular, this PR adds / modifies the following:

  • Removes the require_accelerator marker from test_keep_in_fp32_modules because there's no computation being run there. CPU is enough for that test.
  • Layerwise casting + _keep_in_fp32_modules

@github-actions github-actions Bot added tests size/M PR with diff < 200 LOC labels Aug 6, 2026
@sayakpaul
sayakpaul requested a review from DN6 August 6, 2026 15:26
@sayakpaul
sayakpaul requested a review from dg845 August 12, 2026 07:10
Comment on lines +551 to +553
# Lives here rather than next to the other layerwise casting tests because it asserts
# `_keep_in_fp32_modules` semantics (`enable_layerwise_casting` folds it into the skip patterns) and needs
# no accelerator, while the layerwise casting mixin is accelerator-gated.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this comment might be a little inaccurate as LayerwiseCastingTesterMixin doesn't have a @require_accelerator decorator, but MemoryTesterMixin does. (But I think we always use MemoryTesterMixin in practice, so maybe it's a moot point.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think we always use MemoryTesterMixin

I didn't get this point, sorry.

@dg845 dg845 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Left one small comment.

@sayakpaul
sayakpaul merged commit 9558d91 into main Aug 13, 2026
17 of 18 checks passed
@sayakpaul
sayakpaul deleted the tighten-keep32-modules branch August 13, 2026 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M PR with diff < 200 LOC tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants